Export defines PUBLIC so that headers work properly #250
Conversation
| ) | ||
|
|
||
| if(COMPILER_SUPPORTS_FLOAT128) | ||
| # TODO: Not supported for LLVM bitcode gen as it has a specific compilation flags | ||
| target_sources(${TARGET_LIBSLEEF} PRIVATE sleefqp.c) | ||
| target_compile_definitions(${TARGET_LIBSLEEF} | ||
| PRIVATE ENABLEFLOAT128=1 ${COMMON_TARGET_DEFINITIONS}) |
There was a problem hiding this comment.
COMMON_TARGET_DEFINITIONS was removed from this line because it is already added in the previous target_compile_definitions
|
Hi @xsacha , could you please explain what are you trying to fix here? You say that "sleef.h doesn't work properly". What do you mean by that? What is this patch allowing you to do with sleef.h that was not possible to do before? |
|
What I mean is that if you include the header, it will not have the same defines that were used at compile time. |
Hi , thank you for explaining, but this patch is still not cleat to me. Apologies for asking you to explain more, but I am not a cmake expert and I really don't see why we should export those defines. |
|
Any defines provided through the CMake are not sent when linking the project unless you use the correct CMake functions. PUBLIC is INTERFACE + PRIVATE combined. |
Does this mean that you need to gives visibility to those defines to other CMake projects that include SLEEF as a subproject? Is this what you mean by "externally link"? All these defines are doing is just selecting how to recompile the sources, why would an external project need to see them? |
|
Because when I tried to use sleef headers, it threw undefined linker errors because variables such as avx and float128 were defined at compile time but not when I linked to it in CMake. This commit fixed these errors. |
|
@fpetrogalli How about this patch? I think this patch is safe to merge. |
COMMON_TARGET_DEFINITIONS needs to be PUBLIC on the built libraries so that it is exported to the sleefConfig.cmake.
Otherwise, the sleef.h header does not work properly.
SLEEF_STATIC_LIBS and ENABLEFLOAT128 are two examples that need to be exported. I am unsure if all defines need to be exported but currently the defines are not distinguished in any way.